feat(middleware-logging): implement http.Hijacker #835
Conversation
|
nice! |
|
WDYT @t0rr3sp3dr0 ? |
Yes it is ;) I really tried to add a test for websocket and UPGRADE header but without success (lack of real comprehension of current testing). |
|
I think that we will need a special test case here, something like:
Could this approach work? If yes, I'll try to include a test like it soon |
| @@ -46,3 +49,12 @@ func (lrw *loggingResponseWriter) WriteHeader(statusCode int) { | |||
|
|
|||
| lrw.statusCode = statusCode | |||
| } | |||
|
|
|||
| // implements http.hijacker | |||
There was a problem hiding this comment.
Small change to force loggingResponseWriter to implement http.Hijacker
| // implements http.hijacker | |
| var _ http.Hijacker = (*loggingResponseWriter)(nil) | |
There was a problem hiding this comment.
Please implement a unit test to make sure we always call the underlying Hijacker method when downstreamResponseWriter implements it and another one to check the error returned when downstreamResponseWriter is not an http.Hijacker.
Also, is it safe to implement Hijack and return an error when downstreamResponseWriter is not an http.Hijacker? I'm reading the documentation and we may have problems when using HTTP/2:
The default ResponseWriter for HTTP/1.x connections supports Hijacker, but HTTP/2 connections intentionally do not. ResponseWriter wrappers may also not support Hijacker. Handlers should always test for this ability at runtime.
We need to add some HTTP/2 tests to ensure implementing this method and returning an error doesn't break anything. A code that checks whether or not HTTP.Hijacker is implemented may expect Hijack always to succeed, and we now return an error.
|
Nice!! i validate this with our app and it fixed websocket.... |
Nice to know it! Even that, we do need to include some test that ensures this will continue working during the time. @guilhem do you have any update? If the OP doesn't continue the PR, are you willing to do it @hergi2004 ? |
|
If you are willing to add Playwright to run the e2e test suite for this feature, I could implement a node server and have Playwright verify bidirectional communication is possible over WebSockets when HTTPScaledObject is in play. Perhaps we can skip the middleware if Upgrade: websocket header is not present. |
|
We prefer to manage all the e2e suite using go for orchestation. Could this be feasible for you? |
|
Any update about this? |
|
I too am very interested in this merge request. Please let us know if there is anything we can do to assist moving it forward. |
|
@tghastings , e2e test are pending yet. as the PR seems abandoned, are you willing to open another PR from here adding the e2e tests? |
|
I fully agree with @t0rr3sp3dr0 comment: #835 (review) We have to cover those questions with tests (unit + e2e) |
|
My company has a need for this as well, any updates? |
Sadly the OP hasn't answered :( Are you willing to continue from this PR? The code looks well, but we need to cover it with tests to ensure that we don't break anything |
|
This is a much needed feature. Any update on this PR? |
|
I did a rebase yesterday. |
Did you able to rebase and push |
http.Hijacker is mandatory to forward websocket connexions Signed-off-by: Guilhem Lettron <[email protected]>
|
could you include e2e test coverage for this? |
|
Seema like new changes are breaking the external-scaler deployment. |
http.Hijacker is mandatory to forward websocket connexions
Provide a description of what has been changed
Checklist
README.mddocs/directoryFixes #618 #654